Skip to content

gpui: Filter out NoAction bindings from pending input #30260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented May 8, 2025

This prevents the 1 second delay happening on input when all of the pending bindings are NoAction

Closes #30259

Release Notes:

  • Fixed unnecessary delay when typing a multi-stroke binding that doesn't match any non-null bindings

Copy link

cla-bot bot commented May 8, 2025

We require contributors to sign our Contributor License Agreement, and we don't have @joe-p on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@joe-p
Copy link
Contributor Author

joe-p commented May 8, 2025

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label May 8, 2025
Copy link

cla-bot bot commented May 8, 2025

The cla-bot has been summoned, and re-checked this pull request!

@maxdeviant maxdeviant changed the title fix: filter out NoAction bindings from pending input gpui: Filter out NoAction bindings from pending input May 8, 2025
@joe-p joe-p force-pushed the fix/pending_no_action_bindings branch from 1020c39 to ede008a Compare May 8, 2025 20:34
@joe-p
Copy link
Contributor Author

joe-p commented May 8, 2025

Apologies for the noise. Should have made the PR draft since the first commit was way over-engineered. Now that I understand the codebase a bit more ede008a is a pretty elegant solution.

Tomorrow I can add a unit test but let me know if you think any other tests would be required.

@joe-p
Copy link
Contributor Author

joe-p commented May 9, 2025

In e3985d6 I added a test for a scenario that wasn't initially covered, but that has been fixed in 95a1a86

@ConradIrwin
Copy link
Member

Thanks for this, and sorry for the glacial review time.

The change looks good to me (I spent a bit of time trying to see if we can refactor the loop to make it more obvious why it's right, but couldn't get something obviously better :D).

Now to see if I can get CI to run.

@joe-p
Copy link
Contributor Author

joe-p commented Jun 2, 2025

All good! Yeah it's definitely not something that's immediately intuitive (as shown evident by my many attempts before force pushing).

Part of the initial confusion for me was simply the wording choice of pending over something a bit more descriptive like partial_stroke_match. This was exacerbated by match_keystrokes only returning Some for partial matches but NOT for full matches. I think verbiage along the lines of partial_keystroke_match and full_keystroke_match (and only_partially_matches_keystrokes()) would be a bit easier to make sense of, but that seems outside the scope of this PR.

@ConradIrwin ConradIrwin merged commit 3f90bc8 into zed-industries:main Jun 2, 2025
19 checks passed
@ConradIrwin
Copy link
Member

Makes sense, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-stroke input delay despite lowest context binding being null
2 participants